Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ExitCodeException smart constructors #80

Merged

Conversation

9999years
Copy link
Contributor

@9999years 9999years commented Aug 15, 2024

It's impossible to construct an ExitCodeException manually because the pConfig field accessor is not public. This patch adds two smart constructors,
exitCodeExceptionWithOutput and exitCodeExceptionNoOutput, to fill this use cases.

@9999years 9999years force-pushed the make-exit-code-exception-constructable branch from b1f0ff3 to 2fc9f75 Compare August 15, 2024 18:05
@tomjaguarpaw
Copy link
Collaborator

I'm pretty confused about this. Is it correct that prior to this MR that when ExitCodeExceptions are constructed they never have anything in their eceStdout and eceStderr? That is, there's only one place where one is created, and there those fields are set to empty?

What is the point of those fields then? And what's the use case for populating them?

@tomjaguarpaw
Copy link
Collaborator

This may need @snoyberg to opine.

@9999years
Copy link
Contributor Author

Ah, I got this wrong — documentation on which functions include stdout/stderr was actually added by my CTO in #22! I didn't notice because it uses the record update syntax so my grep didn't turn it up:

checkExitCodeSTM p `catchSTM` \ece -> throwSTM ece
{ eceStdout = stdout
, eceStderr = stderr
}

@9999years 9999years force-pushed the make-exit-code-exception-constructable branch from 2fc9f75 to 316bedc Compare August 15, 2024 22:09
@tomjaguarpaw
Copy link
Collaborator

Please rebase on master to pick up some CI fixes.

`ExitCodeException`s thrown by `typed-process` never populate the
`eceStdout` or `eceStderr` fields, and it's impossible to construct an
`ExitCodeException` manually because the `pConfig` field accessor is not
public. This patch adds two smart constructors,
`exitCodeExceptionWithOutput` and `exitCodeExceptionNoOutput`, to fill
these use cases.
This enables third-parties to construct their own `ExitCodeException`s
from `Process` values.
@9999years 9999years force-pushed the make-exit-code-exception-constructable branch from 316bedc to c4505b1 Compare August 16, 2024 17:26
@9999years
Copy link
Contributor Author

Done, PTAL

@tomjaguarpaw
Copy link
Collaborator

I'll have to defer to @snoyberg on this. I don't really understand the possible consequences.

@snoyberg
Copy link
Member

I don't see anything concerning here myself. No objection to a merge, is there something in particular you were concerned with @tomjaguarpaw?

@tomjaguarpaw
Copy link
Collaborator

I didn't feel comfortable approving myself because I'm not familiar with STM, and also because there is a large range of points in the design space where we should slot in something like this. For example, instead considering these as "creating a value of type ExitCodeException" we could consider them as alternatives to waitExitCode that return ExitCodeExceptions instead of ExitCodes. Alternatively we could just add something slightly different, like

runProcessExitCodeException :: MonadIO m => ProcessConfig stdin stdout stderr -> m ExitCodeException

as a counterpart to runProcess.

Anyway, if you're happy with it then let's go ahead and merge.

@snoyberg snoyberg merged commit 3109782 into fpco:master Aug 19, 2024
23 checks passed
@9999years 9999years deleted the make-exit-code-exception-constructable branch August 19, 2024 16:35
@tomjaguarpaw
Copy link
Collaborator

This has been released in https://hackage.haskell.org/package/typed-process-0.2.12.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants